statistics: reduce allocation of types.Context #52615

winoros · 2024-04-15T12:16:21Z

What problem does this PR solve?

Issue Number: close #52342

Problem Summary:

We still have #52429, but we can reduce the allocations first.

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

codecov · 2024-04-15T12:51:38Z

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 55.6253%. Comparing base (40c81c0) to head (3a4f760).

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #52615         +/-   ##
=================================================
- Coverage   72.3860%   55.6253%   -16.7607%     
=================================================
  Files          1482       1593        +111     
  Lines        428754     598394     +169640     
=================================================
+ Hits         310358     332859      +22501     
- Misses        99088     242580     +143492     
- Partials      19308      22955       +3647

Flag	Coverage Δ
integration	`36.9334% <77.7777%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`49.8961% <ø> (+8.7287%)`	⬆️

pkg/statistics/handle/bootstrap.go

pkg/statistics/handle/handle.go

Rustin170506 · 2024-04-18T09:03:40Z

/cc

Rustin170506

Do you have any test results? Could you please put it on GitHub?

Rustin170506 · 2024-04-18T09:14:44Z

pkg/domain/domain.go

+		// Invalid date values may be inserted into table under some relaxed sql mode. Those values may exist in statistics.
+		// Hence, when reading statistics, we should skip invalid date check. See #39336.
+		sc := ctx.GetSessionVars().StmtCtx
+		sc.SetTypeFlags(sc.TypeFlags().WithIgnoreInvalidDateErr(true).WithIgnoreZeroInDate(true))


Could we use a function for it? Then we don't need to copy the comments and code again in other places.

Do you have any good ideas?
I'm not sure how to make it better(

Rustin170506 · 2024-04-18T09:15:55Z

pkg/session/session.go

@@ -1838,6 +1838,7 @@ func (s *session) useCurrentSession(execOption sqlexec.ExecOption) (*session, fu
 	prevSQL := s.sessionVars.StmtCtx.OriginalSQL
 	prevStmtType := s.sessionVars.StmtCtx.StmtType
 	prevTables := s.sessionVars.StmtCtx.Tables
+	prevTypeFlags := s.sessionVars.StmtCtx.TypeFlags()


Why do we need this change? Could you explain a little bit?

The useCurrentSession will return a func as the clean func after the session executes a SQL.
We need to make sure that the CLEAN operation will not clean the flag we set.

Rustin170506 · 2024-04-18T09:20:49Z

pkg/statistics/histogram.go

@@ -1126,51 +1124,6 @@ func GetIndexPrefixLens(data []byte, numCols int) (prefixLens []int, err error)
 	return prefixLens, nil
 }

-// ExtractTopN extracts topn from histogram.


Good catch!

winoros · 2024-04-18T14:11:32Z

Do you have any test results? Could you please put it on GitHub?

The test env is under rebuilding(

This reverts commit d30ac27.

This reverts commit 59d38e6.

This reverts commit 917ae39.

This reverts commit 93cf601.

This reverts commit 39caa08.

This reverts commit d97bdad.

time-and-fate

Please note that not all places use flags with FlagIgnoreInvalidDateErr and FlagIgnoreZeroInDateErr.
Others LGTM.

ti-chi-bot · 2024-04-28T19:00:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei, time-and-fate

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/statistics/OWNERS~~ [hawkingrei,time-and-fate]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-04-28T19:00:21Z

[LGTM Timeline notifier]

Timeline:

2024-04-15 16:38:41.674694488 +0000 UTC m=+269740.785740938: ☑️ agreed by hawkingrei.
2024-04-28 19:00:20.437909212 +0000 UTC m=+210974.195044785: ☑️ agreed by time-and-fate.

winoros · 2024-04-28T19:05:38Z

/retest

winoros · 2024-04-28T19:42:49Z

/cherrypick release-8.1

ti-chi-bot · 2024-04-28T19:43:31Z

@winoros: new pull request created to branch release-8.1: #52963.

In response to this:

/cherrypick release-8.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

close #52342

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels Apr 15, 2024

winoros force-pushed the reduce-alloc-typectx branch 2 times, most recently from 293bc35 to c648886 Compare April 15, 2024 13:51

hawkingrei reviewed Apr 15, 2024

View reviewed changes

pkg/statistics/handle/bootstrap.go Outdated Show resolved Hide resolved

hawkingrei reviewed Apr 15, 2024

View reviewed changes

pkg/statistics/handle/handle.go Outdated Show resolved Hide resolved

winoros force-pushed the reduce-alloc-typectx branch from c648886 to 6ada327 Compare April 15, 2024 13:59

hawkingrei approved these changes Apr 15, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 15, 2024

ti-chi-bot bot requested a review from Rustin170506 April 18, 2024 09:03

Rustin170506 reviewed Apr 18, 2024

View reviewed changes

winoros added 13 commits April 28, 2024 23:41

statistics: reduce allocation of types.Context

d97bdad

relax the time check for related sctx

39caa08

statistics: reduce the unnecessary allocations of context

93cf601

fix lint and add intest check

917ae39

fix unexpected reset

59d38e6

move a closer place

d30ac27

Revert "move a closer place"

8b28834

This reverts commit d30ac27.

Revert "fix unexpected reset"

7ff2f95

This reverts commit 59d38e6.

Revert "fix lint and add intest check"

2157a82

This reverts commit 917ae39.

Revert "statistics: reduce the unnecessary allocations of context"

0068921

This reverts commit 93cf601.

Revert "relax the time check for related sctx"

673dcb5

This reverts commit 39caa08.

Revert "statistics: reduce allocation of types.Context"

60e510d

This reverts commit d97bdad.

use another way

b6faed1

winoros force-pushed the reduce-alloc-typectx branch from 7c679bd to b6faed1 Compare April 28, 2024 15:55

ti-chi-bot bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 28, 2024

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved labels Apr 28, 2024

fix bazel_prepare

3a4f760

winoros force-pushed the reduce-alloc-typectx branch from 89487df to 3a4f760 Compare April 28, 2024 16:38

time-and-fate approved these changes Apr 28, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 28, 2024

ti-chi-bot bot merged commit 1d698c7 into pingcap:master Apr 28, 2024
21 of 23 checks passed

winoros deleted the reduce-alloc-typectx branch April 28, 2024 19:42

ti-chi-bot mentioned this pull request Apr 28, 2024

statistics: reduce allocation of types.Context (#52615) #52963

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Apr 29, 2024

statistics: reduce allocation of types.Context (#52615) (#52963)

f37e5f3

close #52342

winoros mentioned this pull request Oct 8, 2024

tidb throws error when querying zero datetime with table statistics #56480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: reduce allocation of types.Context #52615

statistics: reduce allocation of types.Context #52615

winoros commented Apr 15, 2024

codecov bot commented Apr 15, 2024 •

edited

Loading

Rustin170506 commented Apr 18, 2024

Rustin170506 left a comment

Rustin170506 Apr 18, 2024

winoros Apr 18, 2024

Rustin170506 Apr 18, 2024

winoros Apr 25, 2024

Rustin170506 Apr 18, 2024

winoros commented Apr 18, 2024

time-and-fate left a comment

ti-chi-bot bot commented Apr 28, 2024

ti-chi-bot bot commented Apr 28, 2024

winoros commented Apr 28, 2024

winoros commented Apr 28, 2024

ti-chi-bot commented Apr 28, 2024

statistics: reduce allocation of types.Context #52615

statistics: reduce allocation of types.Context #52615

Conversation

winoros commented Apr 15, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Apr 15, 2024 • edited Loading

Codecov Report

Rustin170506 commented Apr 18, 2024

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Apr 18, 2024

Choose a reason for hiding this comment

winoros Apr 18, 2024

Choose a reason for hiding this comment

Rustin170506 Apr 18, 2024

Choose a reason for hiding this comment

winoros Apr 25, 2024

Choose a reason for hiding this comment

Rustin170506 Apr 18, 2024

Choose a reason for hiding this comment

winoros commented Apr 18, 2024

time-and-fate left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Apr 28, 2024

ti-chi-bot bot commented Apr 28, 2024

[LGTM Timeline notifier]

winoros commented Apr 28, 2024

winoros commented Apr 28, 2024

ti-chi-bot commented Apr 28, 2024

codecov bot commented Apr 15, 2024 •

edited

Loading